Add support for External Keystone Service #653
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dmendiza The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/0636e0d41791487589955f58fe17b071 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 57s |
dmendiza
force-pushed
the
external-keystone
branch
from
3ccee68 to
4ebeb65
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/aac4c90d99a04ad1beac31fb4502f812 ❌ openstack-k8s-operators-content-provider FAILURE in 9m 33s |
dmendiza
force-pushed
the
external-keystone
branch
from
4ebeb65 to
227b00b
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/13d740041bf24217a22f1de016ce4efa ❌ openstack-k8s-operators-content-provider FAILURE in 9m 05s |
This patch adds a new `ExternalKeystoneAPI` property to KeystoneAPI to enable the use of an existing Keystone Service that is external to the OpenShift environment used to run this operator. For example, a multi-region deployment where one region is running a centralized Keystone service can use this to deploy additional regions that can use the centralized Keystone service without the need to run their own instance of Keystone. Assisted-by: Cursor (Auto Model)
dmendiza
force-pushed
the
external-keystone
branch
from
227b00b to
0e4e747
Compare
| ) | ||
|
|
||
| var ( | ||
| // interfaceBundleKeys maps endpoint winterfaces to their corresponding key in the CA bundle secret |
There was a problem hiding this comment.
s/winterfaces/interfaces/
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ef704189ca0d4a08ac566e2756bb91c0 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 57s |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/b5471949529848f0aae25cd0855f98aa ❌ openstack-k8s-operators-content-provider FAILURE in 9m 21s |
|
recheck |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/8e97f5c52be1434c822b2bc4304b1a95 ❌ openstack-k8s-operators-content-provider FAILURE in 9m 16s |
|
/recheck |
|
/test keystone-operator-build-deploy-kuttl |
Generate the clouds.yaml for the External Keystone API.
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/15db4974754f4f8dbeab5a62c88255c3 ❌ openstack-k8s-operators-content-provider FAILURE in 10m 02s |
Refactor the change added in this branch to pick the right bundle internally based on KeystoneAPI spec instead of making callers of GetAdminServiceClient figure that out. The client will continue to default to the internal interface, but use the public interface when using an external Keytone API.
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/63d602ef9db54a5e850cc9eac9415232 ❌ openstack-k8s-operators-content-provider FAILURE in 11m 38s |
|
recheck |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/2385a873c8954c189a1fdf321f50559f ❌ openstack-k8s-operators-content-provider FAILURE in 9m 22s |
|
recheck |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/5b01fa6733d54721be36fcdf0bd1d8ae ❌ openstack-k8s-operators-content-provider FAILURE in 12m 58s |
|
recheck |
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
|
recheck |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/b33d251fb2134a3ba4cac89f85a22261 ❌ openstack-k8s-operators-content-provider FAILURE in 23m 54s |
|
/test |
|
@vakwetu: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test keystone-operator-build-deploy-kuttl |
Removed the Result object from the helper fuction return value and refactored to return the error (or nil) to let the calling reconcile function figure out what to do with the error. This restores the original logic that was modified by the initial refactor. Previous to this patch the reconcile functions would continue to execute when the TLS secret was not found.
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/8d6ea84176b34abe923a571f8be1062a ❌ openstack-k8s-operators-content-provider TIMED_OUT in 31m 20s |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f1efc6f1b3bd455d801b2ff33d9842b4 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 45s |
- Add test to verify region field is set in status when specified - Add test for external keystone API functionality including: * Region field being set in status * API endpoints being correctly set from override spec * No deployment being created for external keystone API * Conditions being set correctly - Fix timing issue in webhook test by adding Eventually wrapper All 70 functional tests now pass.
vakwetu
force-pushed
the
external-keystone
branch
from
6fc2864 to
a731b7f
Compare
vakwetu
changed the title
| instance.Status.APIEndpoints = map[string]string{} | ||
| } | ||
| // Copy endpoints from spec to status | ||
| for endpointType, data := range instance.Spec.Override.Service { |
There was a problem hiding this comment.
Hi @dmendiza thanks for this patch. While I initially thought that we didn't need a boolean to drive this behavior, I took a step back and I think this is more clear to express the intent of relying on an external keystone instance.
So +1 to have a .Spec.ExternalKeystoneAPI field that by default is set to false.
Of course if the boolean is true but we have no overrides (or no endpoints defined) we land to condition.InputReadyWaitingMessage and the reconciliation loop is re-kicked.
At this point we need manual intervention (edit the CR and properly set the endpoint overrides) to unblock the reconciliation and eventually converge into a Ready state.
I'm wondering though if we should fail earlier in the process, and provide webhooks that would prevent the reconciliation to start from the very beginning, so we can optimize how the user input is handled.
By having webhooks we could do early validation on both Create and Update (e.g. see here [1]) and force the user to provide the required endpoints override.
In api/v1beta1/keystoneapi_webhook.go we could have something like:
func (r *KeystoneAPI) validateExternalKeystoneAPI() error {
if r.Spec.ExternalKeystoneAPI {
if r.Spec.Override.Service == nil {
return fmt.Errorf("external Keystone API requires service override configuration")
}
// Extract and validate endpoints from service overrides
... <more logic> ....
}and prevent the reconciliation to start based on the CR definition (the same validation function could be called from validateUpdate as well).
| // Copy endpoints from spec to status | ||
| for endpointType, data := range instance.Spec.Override.Service { | ||
| if data.EndpointURL != nil { | ||
| instance.Status.APIEndpoints[string(endpointType)] = *data.EndpointURL |
There was a problem hiding this comment.
Are we always defining both public and internal endpoints? Should that need to be validated (the webhooks topic might be back here).
I see a potential issue when one of them (either public or internal) is missing from the overrides. I assume that because EndpointURL is not nil and you have at least 1 endpoint the Status get populated, but when a service looks for the missing one (see glance for example [1]), we fail rendering the template (see [2] for example).
This goes under the assumption that services entirely trust keystone to get endpoints [3], and fail if any required info is missing.
Maybe I'm overlooking something but in this case even setting the two urls to the same endpoint is something we might want to ensure (and it would avoid adding complexity to all the services like glance that might need to be patched to not fail and provide a more solid implementation).
Note that I didn't try this patch to prove my assumption, but seems something we might want to ensure to avoid an unwanted behavior.
[1] https://github.com/openstack-k8s-operators/glance-operator/blob/main/internal/controller/glanceapi_controller.go#L1226-L1233
[2] https://github.com/openstack-k8s-operators/glance-operator/blob/main/templates/common/config/00-config.conf#L42
[3] https://github.com/openstack-k8s-operators/keystone-operator/blob/main/api/v1beta1/keystoneapi_types.go#L314
| return ctrlResult, nil | ||
| } | ||
|
|
||
| // service DB is not needed |
There was a problem hiding this comment.
I would have collected L796-817 into a setExternalKeystoneConditions to explicitly handle conditions (and potentially address any related logic) in a dedicated function, but it's really a minor (probably more cosmetic) thing, so we can keep as it is.
There was a problem hiding this comment.
@vakwetu @dmendiza I have another comment here, that invalidates my previous one.
Instead of marking all the conditions as True, which is fine but misleading because you don't need them, would it make more sense to setup them conditionally like we did for topology here [1]?
// Add conditions only for internal Keystone API
if !instance.Spec.ExternalKeystoneAPI {
cl.Set(condition.UnknownCondition(condition.DBReadyCondition, condition.InitReason, condition.DBReadyInitMessage))
cl.Set(condition.UnknownCondition(condition.DBSyncReadyCondition, condition.InitReason, condition.DBSyncReadyInitMessage))
// ... others ...
}In other words if instance.Spec.ExternalKeystoneAPI is false (regular use case) you initialize them, otherwise they can be skipped and you can remove from L797 to L815 (and probably L831 as well).
| Log.Info("Reconciling Service delete") | ||
|
|
||
| // If using external Keystone API, we don't have any resources to clean up | ||
| if instance.Spec.ExternalKeystoneAPI { |
There was a problem hiding this comment.
while not strictly required (because we call it on L521), I think this optimization is justified by the fact that we save a bunch of calls before reaching L521, so I'm not against this extra if.
This commit adds webhook validation to ensure proper configuration when using external Keystone API. The validation: - Requires service override configuration when ExternalKeystoneAPI is true - Ensures both public and internal endpoints are defined - Ensures both endpoints have EndpointURL set This prevents reconciliation from starting with invalid configuration and avoids template rendering failures in services (like Glance) that depend on both endpoints. The validation is integrated into both ValidateCreate and ValidateUpdate webhook functions to catch configuration errors early. Related: PR comments requesting early validation for external Keystone API
|
Added the webhook as requested by @fmount . I'll test the webhook in the morning and then remove the DNM tag. |
api/v1beta1/keystoneapi_webhook.go
Outdated
| if spec.Override.Service == nil || len(spec.Override.Service) == 0 { | ||
| allErrs = append(allErrs, field.Required( | ||
| overridePath, | ||
| "external Keystone API requires service override configuration", |
There was a problem hiding this comment.
if we have a ctlplane keystone section definition like the following:
spec:
.....
keystone:
apiOverride:
route: {}
template:
override:
service:
internal:
metadata:
annotations:
metallb.universe.tf/address-pool: internalapi
metallb.universe.tf/allow-shared-ip: internalapi
metallb.universe.tf/loadBalancerIPs: 172.17.0.80
spec:
type: LoadBalancer
databaseInstance: openstack
secret: osp-secret(I took this snippet as an example from openstack-operator/config/samples, you can technically bypass this check, which seems a more general thing we could check for several reasons). Not saying this if is necessarily wrong, just wanted to point out that maybe len(spec.Override.Service) == 0 is not useful.
I understand that when spec.ExternalKeystoneAPI is enabled we don't care about other overrides, but basically you can put anything to bypass this check with invalid data.
The actual validation happens later: The hasPublic and hasInternal boolean checks are what truly determine if the required endpoint overrides exist.
| instance := GetKeystoneAPI(keystoneAPIName) | ||
| g.Expect(instance).NotTo(BeNil()) | ||
| g.Expect(instance.Status.Conditions.IsTrue(condition.ReadyCondition)).To(BeTrue()) | ||
| }, timeout, interval).Should(Succeed()) |
There was a problem hiding this comment.
wondering if we should have an explicit test here for externalKeystoneAPI validation.
I drafted something to give you the idea of what I had in mind in case you think it's useful:
When("ExternalKeystoneAPI validation", func() {
It("rejects ExternalKeystoneAPI=true with nil service override", func() {
spec := GetDefaultKeystoneAPISpec()
spec["externalKeystoneAPI"] = true
// Don't set any override
raw := map[string]any{
"apiVersion": "keystone.openstack.org/v1beta1",
"kind": "KeystoneAPI",
"metadata": map[string]any{
"name": keystoneAPIName.Name,
"namespace": keystoneAPIName.Namespace,
},
"spec": spec,
}
unstructuredObj := &unstructured.Unstructured{Object: raw}
_, err := controllerutil.CreateOrPatch(
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(
ContainSubstring(
"external Keystone API requires service override configuration"),
)
})
It("rejects ExternalKeystoneAPI=true with empty service override", func() {
spec := GetDefaultKeystoneAPISpec()
spec["externalKeystoneAPI"] = true
spec["override"] = map[string]any{
"service": map[string]any{},
}
raw := map[string]any{
"apiVersion": "keystone.openstack.org/v1beta1",
"kind": "KeystoneAPI",
"metadata": map[string]any{
"name": keystoneAPIName.Name,
"namespace": keystoneAPIName.Namespace,
},
"spec": spec,
}
unstructuredObj := &unstructured.Unstructured{Object: raw}
_, err := controllerutil.CreateOrPatch(
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(
ContainSubstring(
"external Keystone API requires service override configuration"),
)
})
It("rejects ExternalKeystoneAPI=true with service override but no endpoints", func() {
spec := GetDefaultKeystoneAPISpec()
spec["externalKeystoneAPI"] = true
spec["override"] = map[string]any{
"service": map[string]any{
"admin": map[string]any{
"metadata": map[string]any{
"labels": map[string]any{
"custom": "label",
},
},
},
},
}
raw := map[string]any{
"apiVersion": "keystone.openstack.org/v1beta1",
"kind": "KeystoneAPI",
"metadata": map[string]any{
"name": keystoneAPIName.Name,
"namespace": keystoneAPIName.Namespace,
},
"spec": spec,
}
unstructuredObj := &unstructured.Unstructured{Object: raw}
_, err := controllerutil.CreateOrPatch(
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(
ContainSubstring(
"external Keystone API requires public endpoint to be defined"),
)
})
It("rejects ExternalKeystoneAPI=true with missing public endpoint", func() {
spec := GetDefaultKeystoneAPISpec()
spec["externalKeystoneAPI"] = true
spec["override"] = map[string]any{
"service": map[string]any{
"internal": map[string]any{
"endpointURL": "http://internal.keystone.example.com:5000",
},
},
}
raw := map[string]any{
"apiVersion": "keystone.openstack.org/v1beta1",
"kind": "KeystoneAPI",
"metadata": map[string]any{
"name": keystoneAPIName.Name,
"namespace": keystoneAPIName.Namespace,
},
"spec": spec,
}
unstructuredObj := &unstructured.Unstructured{Object: raw}
_, err := controllerutil.CreateOrPatch(
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(
ContainSubstring(
"external Keystone API requires public endpoint to be defined"),
)
})
It("rejects ExternalKeystoneAPI=true with missing internal endpoint", func() {
spec := GetDefaultKeystoneAPISpec()
spec["externalKeystoneAPI"] = true
spec["override"] = map[string]any{
"service": map[string]any{
"public": map[string]any{
"endpointURL": "http://public.keystone.example.com:5000",
},
},
}
raw := map[string]any{
"apiVersion": "keystone.openstack.org/v1beta1",
"kind": "KeystoneAPI",
"metadata": map[string]any{
"name": keystoneAPIName.Name,
"namespace": keystoneAPIName.Namespace,
},
"spec": spec,
}
unstructuredObj := &unstructured.Unstructured{Object: raw}
_, err := controllerutil.CreateOrPatch(
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(
ContainSubstring(
"external Keystone API requires internal endpoint to be defined"),
)
})
It("rejects ExternalKeystoneAPI=true with empty endpointURL", func() {
spec := GetDefaultKeystoneAPISpec()
spec["externalKeystoneAPI"] = true
spec["override"] = map[string]any{
"service": map[string]any{
"public": map[string]any{
"endpointURL": "",
},
"internal": map[string]any{
"endpointURL": "http://internal.keystone.example.com:5000",
},
},
}
raw := map[string]any{
"apiVersion": "keystone.openstack.org/v1beta1",
"kind": "KeystoneAPI",
"metadata": map[string]any{
"name": keystoneAPIName.Name,
"namespace": keystoneAPIName.Namespace,
},
"spec": spec,
}
unstructuredObj := &unstructured.Unstructured{Object: raw}
_, err := controllerutil.CreateOrPatch(
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(
ContainSubstring(
"external Keystone API requires endpointURL to be set for public endpoint"),
)
})
It("accepts ExternalKeystoneAPI=true with valid endpoints", func() {
spec := GetDefaultKeystoneAPISpec()
spec["externalKeystoneAPI"] = true
spec["override"] = map[string]any{
"service": map[string]any{
"public": map[string]any{
"endpointURL": "http://public.keystone.example.com:5000",
},
"internal": map[string]any{
"endpointURL": "http://internal.keystone.example.com:5000",
},
},
}
raw := map[string]any{
"apiVersion": "keystone.openstack.org/v1beta1",
"kind": "KeystoneAPI",
"metadata": map[string]any{
"name": keystoneAPIName.Name,
"namespace": keystoneAPIName.Namespace,
},
"spec": spec,
}
unstructuredObj := &unstructured.Unstructured{Object: raw}
_, err := controllerutil.CreateOrPatch(
th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
Expect(err).NotTo(HaveOccurred())
},
})
})
})(I tried it locally and is seems to validate the new webhook code).
Note that there's no url.Parse validation yet, but it can be easily added in case you think is a useful thing to have.
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f5524f39482c44b091e35abd7a858ae1 ❌ openstack-k8s-operators-content-provider FAILURE in 8m 24s |
This commit enhances the external Keystone API webhook validation by: 1. Adding URL format validation using net/url.Parse() to ensure endpointURL values are valid URLs for both public and internal endpoints. The validation checks both for parsing errors and requires a URL scheme (http:// or https://) to catch URLs without schemes that url.Parse() would otherwise accept. 2. Removing the redundant check for nil or empty service override. As noted in PR comments, this check could be bypassed by providing an empty map or invalid keys. The actual validation that matters is whether hasPublic and hasInternal are both true, which is checked later in the function. This makes the validation more robust and prevents bypassing the check. 3. Adding comprehensive test coverage for external Keystone API validation including: - Rejection when service override is nil (checks for missing endpoints) - Rejection when service override is empty (checks for missing endpoints) - Rejection when only admin endpoint is provided (missing public/internal) - Rejection when public endpoint is missing - Rejection when internal endpoint is missing - Rejection when endpointURL is empty string - Rejection of URLs without scheme for public endpoint - Rejection of malformed URLs for internal endpoint - Acceptance when both public and internal endpoints are valid All 79 tests pass, including 9 new tests for external Keystone API validation. Related: PR comments requesting URL validation, test coverage, and removing bypassable validation checks
vakwetu
force-pushed
the
external-keystone
branch
from
e06fc42 to
18daecb
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/cadf83a91a194f36a70dd09535b46513 ❌ openstack-k8s-operators-content-provider FAILURE in 5m 03s |
|
Added webhook appears to work just fine! Please review! |
|
recheck |
| // serviceLabels? | ||
|
|
||
| // Verify override spec is valid | ||
| if instance.Spec.Override.Service == nil { |
There was a problem hiding this comment.
Is simply checking for nil enough here?
4 participants
This patch adds a new
ExternalKeystoneAPIproperty to KeystoneAPI to enable the use of an existing Keystone Service that is external to the OpenShift environment used to run this operator.For example, a multi-region deployment where one region is running a centralized Keystone service can use this to deploy additional regions that can use the centralized Keystone service without the need to run their own instance of Keystone.
Assisted-by: Cursor (Auto Model)